-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FFIgenPad #1390
base: main
Are you sure you want to change the base?
FFIgenPad #1390
Conversation
A demo of the web app: https://ffigenpad.surge.sh/ |
@dcharkes @eyebrowsoffire I think we are ready of review. |
Hey, I've been subscribed to the PR for a while now, nice work! I noticed there's a complete copy of ffigen in the code, and I was wondering if maybe it can be imported instead so it stays up-to-date with the version on pub.dev? |
@Levi-Lesches thanks for your interest! there are still multiple 'issues' preventing me from doing that, I have listed them on this blog post, in short stuff like loading dylibs and ffi.Structs are not really patchable if you directly import from ffigen. I am also experimenting with a way we can use patch changes to somehow upstream changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these files are copy-pasta'd from the ffigen library (with some modifications). Could you add a comment to the top of each of those that indicates which file it is derived from?
@eyebrowsoffire I made sure to keep the project structure the same (ie |
How about a section of the readme that lists which subsets of the tree are derived from other sources and where to find them? I know you have kept the project structure the same, but even just reviewing this code right now it is not clear to me what is a new file and what is a copied/modified file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay!
|
||
- run: dart pub get | ||
working-directory: pkgs/ffigenpad | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Run dart analyze and dart format on the code on the CI here.
} | ||
} | ||
|
||
// TODO: look into better color formatting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File bugs on dart-lang/native and put the URL in TODO(<url>): <short decription>
.
// Log all headers for user. | ||
_logger.info('Input Headers: ${config.entryPoints}'); | ||
|
||
// TODO: for some reason it can't add to a List of pointers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
|
||
### include/ | ||
|
||
Contains header files for libclang that are used by ffigen to generate dart:ffi bindings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dart:ffi
in backticks.
Is there a script to update include/clang-c/
from the clang build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's needed but I'll add it.
cd web | ||
npm i # install dependencies | ||
npm run build | ||
# preview with: npm run preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
http://localhost:4173/ -> 404
This is a PR for adding FFIgenPad (a GSoC proposal)
@Native
bindings instead of DynamicLibrary cause of dart2wasmClang
class in ffigen respectively, this leads to the only changes in many files being the import pathsFFIgenPad also requires a wasm build of libclang, bundled with wrapper.c, which currently is being tracked with git but there are no instructions on how to setup and build it, I'll probably add it soon though.
Contribution guidelines:
dart format
.Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.